Skip to content

Improve precision of Duration-float operations#150933

Open
eggyal wants to merge 1 commit intorust-lang:mainfrom
eggyal:issue-149794
Open

Improve precision of Duration-float operations#150933
eggyal wants to merge 1 commit intorust-lang:mainfrom
eggyal:issue-149794

Conversation

@eggyal
Copy link
Contributor

@eggyal eggyal commented Jan 10, 2026 •
edited by rustbot
Loading

View all comments

Rather than convert Duration to a float in order to multiply or divide by a floating point number, which entails a loss of precision, we instead represent both operands as u128 nanoseconds and perform integer arithmetic thereupon.

This improvement in precision affects some of the documented examples.

Given that these methods have been stabilised, is it a breaking change to affect their output? If so, this PR obviously cannot be merged as-is: we might instead need to create new methods for this more precise calculation (and possibly deprecate the existing ones).

Fixes #149794
r? libs-api

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 10, 2026
eggyal
eggyal commented Jan 10, 2026
View reviewed changes
quaternic
quaternic reviewed Jan 10, 2026
View reviewed changes
quaternic
quaternic reviewed Jan 10, 2026
View reviewed changes
@eggyal eggyal marked this pull request as draft January 11, 2026 03:09
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@eggyal eggyal force-pushed the issue-149794 branch 2 times, most recently from 5844308 to adc30ec Compare January 11, 2026 15:14
@rust-log-analyzer

This comment has been minimized.

Sign in to view
eggyal
eggyal commented Jan 11, 2026
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@eggyal
Copy link
Contributor Author

eggyal commented Jan 12, 2026

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2026
@eggyal eggyal marked this pull request as ready for review January 12, 2026 03:23
@eggyal eggyal requested a review from quaternic January 12, 2026 03:23
@eggyal eggyal force-pushed the issue-149794 branch 2 times, most recently from cf585ba to c7098b3 Compare January 12, 2026 10:48
@eggyal
Copy link
Contributor Author

eggyal commented Jan 12, 2026

No, some corner cases are not quite right here. I'll add some more tests and push a fix.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2026
@eggyal eggyal marked this pull request as draft January 12, 2026 14:27
@eggyal
Copy link
Contributor Author

eggyal commented Jan 12, 2026

@rustbot ready

@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

eggyal
eggyal commented Mar 3, 2026
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 3, 2026
@JonathanBrouwer
Rollup merge of rust-lang#153358 - eggyal:duration-flop-boundary-test…
1d5bbbd 
…s, r=tgross35

Boundary tests for various Duration-float operations

As requested in rust-lang#150933 (comment)

r? tgross35
Zalathar added a commit to Zalathar/rust that referenced this pull request Mar 4, 2026
@Zalathar
Rollup merge of rust-lang#153358 - eggyal:duration-flop-boundary-test…
54bb7b8 
…s, r=tgross35

Boundary tests for various Duration-float operations

As requested in rust-lang#150933 (comment)

r? tgross35
rust-timer added a commit that referenced this pull request Mar 4, 2026
@rust-timer
Unrolled build for #153358
19bee38 
Rollup merge of #153358 - eggyal:duration-flop-boundary-tests, r=tgross35

Boundary tests for various Duration-float operations

As requested in #150933 (comment)

r? tgross35
@tgross35
Copy link
Contributor

tgross35 commented Mar 4, 2026

I don't expect it to tell us that much, but I think this works enough to kick off a crater run.

@bors try

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Mar 4, 2026
@bors
Auto merge of #150933 - eggyal:issue-149794, r=<try>
8da2082 
Improve precision of Duration-float operations
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 4, 2026

☀️ Try build successful (CI)
Build commit: 8da2082 (8da20828b6ea601dfd8e2448cb50ace88876bd78, parent: d933cf483edf1605142ac6899ff32536c0ad8b22)

@tgross35
Copy link
Contributor

tgross35 commented Mar 4, 2026

@craterbot run mode=build-and-test

@craterbot
Copy link
Collaborator

craterbot commented Mar 4, 2026

👌 Experiment pr-150933 created and queued.
🤖 Automatically detected try build 8da2082
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2026
@eggyal
Copy link
Contributor Author

eggyal commented Mar 4, 2026

Should we run crater while our own tests are failing? Surely I should fix that first!

@tgross35
Copy link
Contributor

tgross35 commented Mar 4, 2026

The failure is just a changed panic string right? I don't expect anyone to be checking that

@eggyal
Copy link
Contributor Author

eggyal commented Mar 4, 2026

We also were introducing new panics for some negative factors that previously yielded zero, which has been corrected in my latest push.

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2026
@eggyal
Improve precision of Duration-float operations
48a261c 
Rather than convert Duration to a float in order to multiply or divide
by a floating point number, which entails a loss of precision, we
instead represent both operands as u128 nanoseconds and perform integer
arithmetic thereupon.
@Amanieu
Copy link
Member

Amanieu commented Mar 4, 2026

Once you get into floating-point arithmetic, some amount of precision loss is to be expected. I actually don't find it too surprising that multiplying by 1.0 would result in precision loss for very large durations. The main problem with the code in this PR is that it adds a huge amount of complexity for what is really an edge case. It would be more appropriate to just have a warning in the documentation which explains that this method may cause precision loss due to the use of floating-point arithmetic, even when multiplying with 1.0.

@rfcbot close libs

@rust-rfcbot
Copy link
Collaborator

rust-rfcbot commented Mar 4, 2026 •
edited by cuviper
Loading

Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rust-rfcbot rust-rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 4, 2026
@Amanieu Amanieu removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Mar 4, 2026
@rust-rfcbot rust-rfcbot added the disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgross35 tgross35 tgross35 left review comments

@quaternic quaternic Awaiting requested review from quaternic

Assignees

@tgross35 tgross35

Labels

disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-crater Status: Waiting on a crater run to be completed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Loss of precision in Duration::mul_f32 (and mul_f64)

9 participants

@eggyal @rustbot @rust-log-analyzer @tgross35 @craterbot @Amanieu @rust-rfcbot @quaternic @BurntSushi